Add D-Bus API for configuring NTP servers of systemd-timesyncd #207
Conversation
sairon
changed the title
Add new `time` package that currently provides methods that configure
NTP and FallbackNTP options of systemd-timesyncd service. The service is
restarted when the values are changed. Example usage through gdbus:
gdbus call --system --dest io.hass.os \
--object-path /io/hass/os/Time/Timesyncd \
--method org.freedesktop.DBus.Properties.Set \
io.hass.os.Time.Timesyncd NTPServer \
"<['pool.ntp.org', 'time.google.com']>"
A `lineinfile` helper has been implemented for adjusting Systemd unit
files, the inspiration comes from Ansible's module of the same name,
although the behavior is slightly different (hopefully still quite
intuitive). Unit tests for the core methods handling the file content
are included.
In the future, the `time` package could also handle system timezone and
other time-related tasks.
sairon
force-pushed
the
timesyncd-ntp-config
branch
from
df9f1d4 to
0f09f2b
Compare
|
@coderabbitai full review |
WalkthroughWalkthroughThe changes introduce a new dependency on the Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant DBus
participant TimeSyncD
participant LineInFile
Main->>DBus: InitializeDBus(conn)
DBus->>TimeSyncD: Setup NTP Configuration
TimeSyncD->>LineInFile: Modify NTP Settings
LineInFile-->>TimeSyncD: Return Updated Config
TimeSyncD-->>DBus: Notify Configuration Update
DBus-->>Main: Complete Initialization
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Additional context usedPath-based instructions (1)
Additional comments not posted (16)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| var ( | ||
| optNTPServer []string | ||
| optFallbackNTPServer []string | ||
| configFile = lineinfile.LineInFile{FilePath: timesyncdConf} | ||
| ) |
There was a problem hiding this comment.
Global Variables and Configuration File Initialization
The use of global variables (optNTPServer, optFallbackNTPServer) is generally discouraged as it can lead to issues with state management, especially in a multi-threaded environment. Consider encapsulating these within the timesyncd struct to better manage their scope and lifecycle.
Suggest encapsulating global variables within the timesyncd struct:
-type timesyncd struct {
- conn *dbus.Conn
- props *prop.Properties
+type timesyncd struct {
+ conn *dbus.Conn
+ props *prop.Properties
+ optNTPServer []string
+ optFallbackNTPServer []string
}Committable suggestion was skipped due to low confidence.
| func setNTPServer(c *prop.Change) *dbus.Error { | ||
| servers, ok := c.Value.([]string) | ||
| if !ok { | ||
| return dbus.MakeFailedError(fmt.Errorf("invalid type for NTPServer")) | ||
| } | ||
|
|
||
| value := strings.Join(servers, " ") | ||
|
|
||
| if err := setTimesyncdConfigProperty("NTP", value); err != nil { | ||
| return dbus.MakeFailedError(err) | ||
| } | ||
|
|
||
| optNTPServer = servers | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Function setNTPServer Review
This function handles the setting of NTP servers via D-Bus properties. The error handling and type assertion are correctly implemented. However, the function directly modifies a global variable, which could lead to race conditions or other concurrency issues. Consider using a mutex or similar synchronization mechanism if the API is expected to handle concurrent modifications.
Suggest adding synchronization to handle concurrent access:
+import "sync"
+var mutex sync.Mutex
func setNTPServer(c *prop.Change) *dbus.Error {
+ mutex.Lock()
+ defer mutex.Unlock()
...
}Committable suggestion was skipped due to low confidence.
| func processPresent(inLines []string, params Params) ([]string, error) { | ||
| var outLines []string | ||
|
|
||
| if params.Before != "" && params.After != "EOF" { | ||
| err := fmt.Errorf("cannot specify both Before and After") | ||
| return nil, err | ||
| } | ||
|
|
||
| needsBefore := params.Before != "" | ||
| needsAfter := params.After != "EOF" | ||
| afterRegexp, _ := re.Compile(params.After) | ||
| beforeRegexp, _ := re.Compile(params.Before) | ||
|
|
||
| var beforeIndex = -1 | ||
| var afterIndex = -1 | ||
| var foundIndex = -1 | ||
|
|
||
| for idx, curr := range inLines { | ||
| outLines = append(outLines, curr) | ||
| if needsBefore && beforeIndex < 0 && beforeRegexp.MatchString(curr) { | ||
| beforeIndex = idx | ||
| continue | ||
| } | ||
| if needsAfter && afterIndex < 0 && afterRegexp.MatchString(curr) { | ||
| afterIndex = idx | ||
| continue | ||
| } | ||
| if (needsAfter && afterIndex >= 0 || needsBefore && beforeIndex >= 0) && foundIndex < 0 && params.Regexp.MatchString(curr) { | ||
| foundIndex = idx | ||
| } | ||
| } | ||
|
|
||
| if foundIndex >= 0 { | ||
| // replace found line with the params.Line | ||
| outLines[foundIndex] = params.Line | ||
| } else if params.After == "EOF" { | ||
| outLines = append(outLines, params.Line) | ||
| } else if params.Before == "BOF" { | ||
| outLines = append([]string{params.Line}, outLines...) | ||
| } else if params.After != "" { | ||
| if afterIndex >= 0 { | ||
| // insert after the line matching the After regexp | ||
| outLines = append(outLines[:afterIndex+1], append([]string{params.Line}, outLines[afterIndex+1:]...)...) | ||
| } | ||
| } | ||
|
|
||
| return outLines, nil | ||
| } |
There was a problem hiding this comment.
Review of Line Processing Functions: processPresent and processAbsent
These functions contain complex logic for inserting or removing lines based on the provided parameters. The error handling for conflicting Before and After parameters is correctly implemented. However, the regex compilation inside the loop (lines 113, 114, 162, 163) should be moved outside the loop to optimize performance, as compiling the same regex repeatedly is inefficient.
Consider refactoring to compile the regex expressions once before the loop starts to enhance performance.
- afterRegexp, _ := re.Compile(params.After)
- beforeRegexp, _ := re.Compile(params.Before)
+ var afterRegexp, beforeRegexp *re.Regexp
+ var err error
+ if params.After != "" {
+ afterRegexp, err = re.Compile(params.After)
+ if err != nil {
+ return nil, err
+ }
+ }
+ if params.Before != "" {
+ beforeRegexp, err = re.Compile(params.Before)
+ if err != nil {
+ return nil, err
+ }
+ }Also applies to: 152-190
| func (l LineInFile) Find(regexp string, after string, allowMissing bool) (*string, error) { | ||
| if _, err := os.Stat(l.FilePath); os.IsNotExist(err) { | ||
| if allowMissing { | ||
| return nil, nil | ||
| } | ||
| logging.Error.Printf("File %s does not exist: %s", l.FilePath, err) | ||
| return nil, err | ||
| } | ||
|
|
||
| content, err := os.ReadFile(l.FilePath) | ||
| if err != nil { | ||
| fmt.Print(err) | ||
| return nil, err | ||
| } | ||
|
|
||
| lines := strings.Split(string(content), "\n") | ||
|
|
||
| return processFind(regexp, after, lines), nil | ||
| } | ||
|
|
||
| func processFind(regexp string, after string, inLines []string) *string { | ||
| if inLines == nil { | ||
| return nil | ||
| } | ||
|
|
||
| lineRegexp, _ := re.Compile(regexp) | ||
| afterRegexp, _ := re.Compile(after) | ||
|
|
||
| var foundAfter = false | ||
|
|
||
| for _, curr := range inLines { | ||
| if !foundAfter && afterRegexp.MatchString(curr) { | ||
| foundAfter = true | ||
| continue | ||
| } | ||
| if after != "" && !foundAfter { | ||
| continue | ||
| } | ||
| if lineRegexp.MatchString(curr) { | ||
| return &curr | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Review of Auxiliary Method: Find
The Find method provides functionality to search for a line based on a regex, optionally after a certain line. The implementation handles file existence checks and reads the file content correctly. The separation of the regex matching logic into processFind is a good practice, keeping the Find method clean and focused. However, the error handling could be improved by logging errors consistently as done in other methods.
Enhance error handling by adding logging for all errors in the Find method to maintain consistency with other methods in the file.
- fmt.Print(err)
+ logging.Error.Printf("Error reading file %s: %s", l.FilePath, err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (l LineInFile) Find(regexp string, after string, allowMissing bool) (*string, error) { | |
| if _, err := os.Stat(l.FilePath); os.IsNotExist(err) { | |
| if allowMissing { | |
| return nil, nil | |
| } | |
| logging.Error.Printf("File %s does not exist: %s", l.FilePath, err) | |
| return nil, err | |
| } | |
| content, err := os.ReadFile(l.FilePath) | |
| if err != nil { | |
| fmt.Print(err) | |
| return nil, err | |
| } | |
| lines := strings.Split(string(content), "\n") | |
| return processFind(regexp, after, lines), nil | |
| } | |
| func processFind(regexp string, after string, inLines []string) *string { | |
| if inLines == nil { | |
| return nil | |
| } | |
| lineRegexp, _ := re.Compile(regexp) | |
| afterRegexp, _ := re.Compile(after) | |
| var foundAfter = false | |
| for _, curr := range inLines { | |
| if !foundAfter && afterRegexp.MatchString(curr) { | |
| foundAfter = true | |
| continue | |
| } | |
| if after != "" && !foundAfter { | |
| continue | |
| } | |
| if lineRegexp.MatchString(curr) { | |
| return &curr | |
| } | |
| } | |
| return nil | |
| } | |
| func (l LineInFile) Find(regexp string, after string, allowMissing bool) (*string, error) { | |
| if _, err := os.Stat(l.FilePath); os.IsNotExist(err) { | |
| if allowMissing { | |
| return nil, nil | |
| } | |
| logging.Error.Printf("File %s does not exist: %s", l.FilePath, err) | |
| return nil, err | |
| } | |
| content, err := os.ReadFile(l.FilePath) | |
| if err != nil { | |
| logging.Error.Printf("Error reading file %s: %s", l.FilePath, err) | |
| return nil, err | |
| } | |
| lines := strings.Split(string(content), "\n") | |
| return processFind(regexp, after, lines), nil | |
| } | |
| func processFind(regexp string, after string, inLines []string) *string { | |
| if inLines == nil { | |
| return nil | |
| } | |
| lineRegexp, _ := re.Compile(regexp) | |
| afterRegexp, _ := re.Compile(after) | |
| var foundAfter = false | |
| for _, curr := range inLines { | |
| if !foundAfter && afterRegexp.MatchString(curr) { | |
| foundAfter = true | |
| continue | |
| } | |
| if after != "" && !foundAfter { | |
| continue | |
| } | |
| if lineRegexp.MatchString(curr) { | |
| return &curr | |
| } | |
| } | |
| return nil | |
| } |
| @@ -0,0 +1,175 @@ | |||
| package time | |||
There was a problem hiding this comment.
To avoid clash with the go time package, I'd go for something else, like timesync.
| package time | |
| package timesync |
There was a problem hiding this comment.
I wanted to have the same prefix for the DBus interfaces controlling other time-related features in the future, and so far we've got it synchronized with package naming. In the end the only clash is in the main file, where it can be simply aliased.
There was a problem hiding this comment.
To me this is mostly about filling the gap of missing D-Bus interfaces in systemd. Ideally, we would extend systemd, but this is much more time consuming, so I am fine doing our thing now here.
But, with that in mind, I think it make sense to align with the systemd D-Bus naming: Systemd has two time/date related D-Bus services: org.freedesktop.timesync1 and org.freedesktop.timedate1. What we currently miss is setting the system NTP server, which most likely should be part of the org.freedesktop.timesync1 service.
There was a problem hiding this comment.
Rather than the Systemd D-Bus naming, consider naming of our D-Bus interfaces - now it's io.hass.os.Time.Timesyncd, which is implemented in time/timesyncd.go. If we added e.g. interface for timezone setting (leave aside the fact there's some method in org.freedesktop.timedate1 that might be used instead), a logical path would be io.hass.os.Time.Timezone and the implementation would go to time/timezone.go. If the package should be renamed, then we should also adjust D-Bus paths and interfaces.
There was a problem hiding this comment.
With the other discussion in mind, it kinda seems what we need to build here is mostly a systemd config file manager. So aligning the naming to the config file names probably make sense 🤔
| @@ -0,0 +1,253 @@ | |||
| package lineinfile | |||
There was a problem hiding this comment.
Hm, did you consider using something existing? There is for instance this library: https://github.com/sergeymakinen/go-systemdconf. Not sure if works for the daemon config files, the systemd.syntax man page says it has the same syntax so might work actually.
There was a problem hiding this comment.
Yeah, I did some research there, most of them seemed too bloated, complex and/or unmaintained, so I didn't want to get trapped in the future by introducing such dependency (e.g. this one depends on 5 years old release of a CSS processing library). In the end I went with the "lineinfile" concept, which is powerful enough it's able to drive configuration changes by many Ansible roles in the real world, and it's easy to understand and troubleshoot.
There was a problem hiding this comment.
Hm, yeah unmaintained lib is worse, I agree.
| conn: conn, | ||
| } | ||
|
|
||
| optNTPServer = getNTPServers() |
There was a problem hiding this comment.
I guess this could be a bit racy, depending on when OS Agent/NetworkManager hook gets called 🤔
There was a problem hiding this comment.
The current implementation only touches the persistent configuration file, so NetworkManager hook doesn't affect it in any way. If we want to read the dynamically obtained NTPs written by the NM hook, maybe using a different method/property would be better.
There was a problem hiding this comment.
Why do we even need these getters? There is FallbackNTPServers and SystemNTPServers available on the /org/freedesktop/timesync1 object of the org.freedesktop.timesync1 service.
In a way, this interface to me is mainly a stopgap until systemd-timesyncd has a similar functionality.
There was a problem hiding this comment.
Those can be amalgamation of multiple configuration files as described in man timesyncd.conf. This interface exposes what is statically configured in the file we'd like to be the source of truth for the configuration. I can imagine the "native" Systemd attributes could be used for sanity checking, raising an issue if the configured value doesn't match what's in the config (which e.g. means user has created extra configuration files). But it would be confusing if you had only the setters, and under some circumstances the getters would return different value than what you have set.
I also doubt that Systemd will ever get similar functionality. There are the runtime NTP servers but those we can't use because we need to apply the configuration before our "configuration daemon" (Supervisor) is started. AFAIK Systemd doesn't have a way to edit configuration files through D-Bus API anywhere, and some things can be only configured only through config files.
There was a problem hiding this comment.
Those can be amalgamation of multiple configuration files as described in man timesyncd.conf. This interface exposes what is statically configured in the file we'd like to be the source of truth for the configuration. I can imagine the "native" Systemd attributes could be used for sanity checking, raising an issue if the configured value doesn't match what's in the config (which e.g. means user has created extra configuration files). But it would be confusing if you had only the setters, and under some circumstances the getters would return different value than what you have set.
Right, I definitely would not forward the getter here to D-Bus, that would indeed be confusing.
I am just wonder, if we cannot get away with a more minimal D-Bus interface. Sure, we already have it implemented (with this PR), but there is always maintenance cost.
I also doubt that Systemd will ever get similar functionality. There are the runtime NTP servers but those we can't use because we need to apply the configuration before our "configuration daemon" (Supervisor) is started. AFAIK Systemd doesn't have a way to edit configuration files through D-Bus API anywhere, and some things can be only configured only through config files.
Yeah you probably be right on that there won't be a timesync1 setter. But there is already a systemd-networkd API to set NTP servers:
https://www.freedesktop.org/software/systemd/man/latest/org.freedesktop.network1.html#The%20Manager%20Object
I think systemd sees NTP server configuration a network manager thing, just like DNS. And it should be handled and stored by the network manager, and transferred at the appropriate time (the appropriate link is actually ready). I mean, you can imagine a system where you have two network links, each have their own local NTP servers.. You can't simply mix and match in that scenario. Ofc, this won't be the typical HAOS scenario, but it helps to understand how things develop. On the other hand, the system/fallback NTP's will always be global.
Researching a bit more, I came accross this Cockpit issue: cockpit-project/cockpit#7987 (comment)
And the linked systemd issue: systemd/systemd#7593 (comment) and related systemd/systemd#27469. The gist is that systemd-networkd (and I guess with that systemd-timesyncd) lacks a configuration management API. It is all meant to go through files, and that is what it is currently.
Maybe a generic systemd configuration file management API would be better? 🤔 On the other hand, maybe it is better to have OS Agent as a bit of a firewall for this 😅
There was a problem hiding this comment.
I think systemd sees NTP server configuration a network manager thing, just like DNS. And it should be handled and stored by the network manager, and transferred at the appropriate time (the appropriate link is actually ready). I mean, you can imagine a system where you have two network links, each have their own local NTP servers.
Yes, but I think this is not achievable without using systemd-networkd at the same time. Currently we store the network config in NetworkManager's config files, where we lack any mechanism to save per-link user-configured NTP servers, so we'd have to introduce one (i.e. have our own config files/attributes of individual NM connections that'd somehow propagate to timesyncd config). But the complexity of such implementation would heavily outweigh any maintenance cost of the current approach to this problem.
Anyway, if we shall consider multihomed setups which even require different NTP servers on individual links, then I think this discussion turns academic. AFAIK most of the multihomed setups are not what HA considers anywhere, leave aside the fact that even just the NTP config alone seems too advanced for some 😅 We really should think about the real use case, and the way we want to handle NTP configuration on user's side.
I agree though that we can take a different approach on this problem and instead of exposing "time subsystem" APIs, we could have "system configuration" instead, if that makes more sense to you.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Add new
timepackage that currently provides methods that configure NTP and FallbackNTP options of systemd-timesyncd service. The service is restarted when the values are changed. Example usage through gdbus:gdbus call --system --dest io.hass.os
--object-path /io/hass/os/Time/Timesyncd
--method org.freedesktop.DBus.Properties.Set
io.hass.os.Time.Timesyncd NTPServer
"<['pool.ntp.org', 'time.google.com']>"
A
lineinfilehelper has been implemented to adjust Systemd unit files, the inspiration comes from Ansible's module of the same name, although the behavior is slightly different (hopefully still quite intuitive).In the future, the
timepackage could also handle system timezone and other time-related tasks.Summary by CodeRabbit
New Features
Tests